Skip to content

Logger fallback to console.error if process._rawDebug is missing #592

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Logger fallback to console.error if process._rawDebug is missing #592

wants to merge 5 commits into from

Conversation

waahm7
Copy link
Contributor

@waahm7 waahm7 commented Dec 16, 2024

Duplicate of #590.
When running under Deno instead of NodeJS I get the following stack trace when the native extension is loaded:

N-API call failed: napi_create_threadsafe_function( env, node_rawdebug, NULL, resource_name, 0, 1, ctx, s_threadsafe_log_finalize, ctx, s_threadsafe_log_call, &ctx->log_drain)                                                                                                                                                                           
    @ /codebuild/output/src1849732470/src/aws-crt-nodejs/source/logger.c:277: napi_function_expected                                                   
Fatal error condition occurred in /codebuild/output/src1849732470/src/aws-crt-nodejs/source/logger.c:277: napi_create_threadsafe_function( env, node_rawdebug, NULL, resource
_name, 0, 1, ctx, s_threadsafe_log_finalize, ctx, s_threadsafe_log_call, &ctx->log_drain)                                                                                
Exiting Application

Deno does not provide console._rawDebug so fall back to console.error instead.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@bretambrose
Copy link
Contributor

I don't think we should merge PRs containing code that never gets meaningfully exercised anywhere in testing or CI. I'm also not thrilled about adding support for non-Node execution engines.

@waahm7
Copy link
Contributor Author

waahm7 commented Dec 17, 2024

@bretambrose

I don't think we should merge PRs containing code that never gets meaningfully exercised anywhere in testing or CI. I'm also not thrilled about adding support for non-Node execution engines.

In general, I agree, but for this particular change, _rawDebug is a private function that we should not be using from the Node code [link].

// Most of the time, it's best to use `console.error` to write  
// to the `process.stderr` stream. However, in some cases, such as  
// when debugging the `stream.Writable` class or the `process.nextTick`  
// function, it is useful to bypass JavaScript entirely.

I don't know the Node/JavaScript world well, but I think console.error is recommended over process._rawDebug. Since process._rawDebug is an internal function, Node is free to remove or change its implementation in the future, so it seems reasonable to fall back to console.error if it is not found. I haven't made console.error the default due to performance concerns and not knowing enough to make this change.

I have tested the code in CI to ensure that everything works even if we always use console.error. However, I haven't tested it locally using Deno.

@njbraun njbraun mentioned this pull request Jul 15, 2025
Closed
@njbraun
Copy link

njbraun commented Jul 17, 2025 •
edited
Loading

Completely agree with @waahm7 on this one. This has nothing to do with alternative runtimes and more to do with not using private methods from external codebases. Is there some reason this library has to use this _rawDebug method versus the public methods console.debug/log/error ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@waahm7 @bretambrose @njbraun @nickg